Handle pipe names with whitespace properly #2409
Conversation
| @@ -290,7 +290,7 @@ internal static bool TryCreateServerCore(string clientDir, string pipeName, out | |||
| // The server should be in the same directory as the client | |||
| var expectedCompilerPath = Path.Combine(clientDir, ServerName); | |||
| expectedPath = Environment.GetEnvironmentVariable("DOTNET_HOST_PATH") ?? "dotnet"; | |||
| processArguments = $@"""{expectedCompilerPath}"" {(debug ? "--debug" : "")} server -p {pipeName}"; | |||
| processArguments = $@"""{expectedCompilerPath}"" {(debug ? "--debug" : "")} server -p ""{pipeName}"""; | |||
There was a problem hiding this comment.
Are there other places we manually start a process? Can you double check?
There was a problem hiding this comment.
There was a problem hiding this comment.
Can you double check?
Just did. There is just this and tests.
Should we use https://github.com/aspnet/Common/blob/dev/shared/Microsoft.Extensions.CommandLineUtils.Sources/Utilities/ArgumentEscaper.cs#L24?
I'm not sure actually. I did a quick check and looks like we only use it mostly in BuildTools. Was it intended for using in product code?
There was a problem hiding this comment.
Yes, you can use it in product code. We use it in dotnet-watch.
There was a problem hiding this comment.
Primarily suggesting this since this includes values constructed from user data. There could be other things we might be missing whilst doing double-quote escaping.
There was a problem hiding this comment.
Also, if this project is using the netcoreapp2.1 TFM or higher, use ProcessStartInfo.ArgumentList instead of ProcessStartInfo.Arguments. See for example https://github.com/natemcmaster/dotnet-serve/blob/dac796c44c1c4b7ef101763c60e4d7ddfc3cd2d6/test/dotnet-serve.Tests/DotNetServe.cs#L101-L148.
|
|
||
| // This will no-op if server logging is not enabled. | ||
| ServerLogger.Log(output); | ||
| ServerLogger.Log(error); |
There was a problem hiding this comment.
We've already something similar for the code path when the server is already started. But I missed logging this code path which gets called during the server start.
| @@ -46,7 +46,7 @@ static ServerLogger() | |||
| // Otherwise, assume that the environment variable specifies the name of the log file. | |||
| if (Directory.Exists(loggingFileName)) | |||
| { | |||
| loggingFileName = Path.Combine(loggingFileName, $"server.{loggingFileName}.{GetCurrentProcessId()}.log"); | |||
| loggingFileName = Path.Combine(loggingFileName, $"razorserver.{GetCurrentProcessId()}.log"); | |||
There was a problem hiding this comment.
This change lets me specify absolute path for directories here.
| @@ -54,18 +58,5 @@ public void Dispose() | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| private static string RecursiveFind(string path, string start) | |||
|
🆙 📅 |
| @@ -16,17 +17,31 @@ public static int Main(string[] args) | |||
| var cancel = new CancellationTokenSource(); | |||
| Console.CancelKeyPress += (sender, e) => { cancel.Cancel(); }; | |||
|
|
|||
| var outputWriter = new StringWriter(); | |||
There was a problem hiding this comment.
Minor: the StringWriter is disposable, so should be wrapped with a using statement. However, this is in the main method, which means when it's unused, the process will exit and all the resources will be cleaned up anyway.
ajaybhargavb
force-pushed
the
ajbaaska/pipe-name-space
branch
from
68ab62a to
6497942
Compare
ajaybhargavb
force-pushed
the
ajbaaska/pipe-name-space
branch
from
6497942 to
17e3aa8
Compare
Fixes #2406
The actual bug here was that I was not passing the pipe name in the process arguments within quotes 🤦♂️
@mkArtakMSFT FYI